fix(material/button): combine MatButton and MatAnchor #30492
+110
−209
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crisbeto
requested review from
mmalerba and
andrewseguin
and removed request for
a team
crisbeto
force-pushed
the
combined-buttons
branch
5 times, most recently
from
e1f2211 to
074ed69
Compare
mmalerba
approved these changes
Feb 18, 2025
| @@ -153,14 +162,28 @@ export class MatButtonBase implements AfterViewInit, OnDestroy { | |||
| @Input({transform: booleanAttribute}) | |||
| disabledInteractive: boolean; | |||
|
|
|||
| /** Tab index for the button. */ | |||
| @Input({transform: transformTabIndex}) | |||
| tabIndex: number; | |||
There was a problem hiding this comment.
should this be the deprecated one instead? I think most of our components that take a tabindex use the all-lowercase spelling
There was a problem hiding this comment.
Actually we use tabIndex across a bunch of other components as well. I think it's because the native property is also called tabIndex, see https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/tabIndex
| const classList = (element as HTMLElement).classList; | ||
|
|
||
| this._isAnchor = element.tagName === 'A'; |
There was a problem hiding this comment.
probably should .toUpperCase() this to be safe
There was a problem hiding this comment.
It should be guaranteed to be uppercase. The only time tag names preserve their case is in SVG.
Currently we have two directives for each button variant: `MatButton` which applies to `button` elements and `MatButtonAnchor` which applies to anchors. This is problematic in a couple of ways: 1. The styles, which can be non-trivial, are duplicated if both classes are used. 2. Users have to think about which class they're importing. These changes combine the two classes to resolve the issues and simplify our setup. BREAKING CHANGE: `tabindex` values set as `[attr.tabindex]` set on a Material button might not work as expected. Use `tabindex` for static values, or `[tabindex]`/`[tabIndex]` for dynamic ones.
crisbeto
force-pushed
the
combined-buttons
branch
from
074ed69 to
166d46d
Compare
angular-robot
bot
added
the
detected: breaking change
crisbeto
added
target: major
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we have two directives for each button variant:
MatButtonwhich applies tobuttonelements andMatButtonAnchorwhich applies to anchors.This is problematic in a couple of ways:
These changes combine the two classes to resolve the issues and simplify our setup.
BREAKING CHANGE:
tabindexvalues set as[attr.tabindex]set on a Material button might not work as expected. Usetabindexfor static values, or[tabindex]/[tabIndex]for dynamic ones.